Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add new "MapAction" overloads #30556

Merged
4 commits merged into from
Mar 2, 2021
Merged

Add new "MapAction" overloads #30556

4 commits merged into from
Mar 2, 2021

Conversation

halter73
Copy link
Member

@halter73 halter73 commented Mar 2, 2021 •
edited
Loading

Fixes #30448.

By adding overloads to existing built-in IEndpointRouteBuilder extension methods, we remove the need for the routing attributes ([HttpGet("/"], [HttpPost(..., etc.) in front of most "MapAction" methods. This will be especially handy when the methods become lambdas in non-trivial cases.

namespace Microsoft.AspNetCore.Builder
{
    public static class EndpointRouteBuilderExtensions
    {
        public static MapActionEndpointConventionBuilder MapGet(this IEndpointRouteBuilder endpoints, string pattern, Delegate action);
        public static MapActionEndpointConventionBuilder MapPost(this IEndpointRouteBuilder endpoints, string pattern, Delegate action);
        public static MapActionEndpointConventionBuilder MapPut(this IEndpointRouteBuilder endpoints, string pattern, Delegate action);
        public static MapActionEndpointConventionBuilder MapDelete(this IEndpointRouteBuilder endpoints, string pattern, Delegate action);
        public static MapActionEndpointConventionBuilder Map(this IEndpointRouteBuilder endpoints, string pattern, Delegate action);
        public static MapActionEndpointConventionBuilder Map(this IEndpointRouteBuilder endpoints, RoutePattern pattern, Delegate action);
}

This leaves MapAction for now. I'll have a PR that removes that and IRoutePatternMetadata soon.

@halter73 halter73 requested a review from javiercn as a code owner March 2, 2021 00:31
@ghost ghost added the area-servers label Mar 2, 2021
@halter73 halter73 requested review from davidfowl and pranavkm March 2, 2021 00:31
@halter73 halter73 added area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-minimal-actions Controller-like actions for endpoint routing and removed area-servers labels Mar 2, 2021
@ghost
Copy link

ghost commented Mar 2, 2021

Hello @halter73!

Because this pull request has the auto-merge label, I will be glad to assist with helping to merge this pull request once all check-in policies pass.

p.s. you can customize the way I help with merging this pull request, such as holding this pull request until a specific person approves. Simply @mention me (@msftbot) and give me an instruction to get started! Learn more here.

JamesNK
JamesNK reviewed Mar 2, 2021
View reviewed changes
src/Http/Routing/src/Builder/MapActionEndpointRouteBuilderExtensions.cs Show resolved Hide resolved
src/Http/Routing/src/Builder/MapActionEndpointRouteBuilderExtensions.cs
/// <param name="pattern">The route pattern.</param>
/// <param name="action">The delegate executed when the endpoint is matched.</param>
/// <returns>A <see cref="IEndpointConventionBuilder"/> that can be used to further customize the endpoint.</returns>
public static MapActionEndpointConventionBuilder MapGet(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Aren't there already Map* extension methods on route builder? Do these not colide because of Delegate parameter?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nope.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These are overloads, but they should only be used for delegates that aren't RequestDelegates. This was also mentioned in dotnet/csharplang#4451 (comment).

@ghost ghost merged commit 5207cad into main Mar 2, 2021
@ghost ghost deleted the halter73/30448 branch March 2, 2021 07:49
@halter73 halter73 mentioned this pull request Mar 2, 2021
Merged
pranavkm
pranavkm reviewed Mar 2, 2021
View reviewed changes
src/Http/Routing/src/Builder/MapActionEndpointRouteBuilderExtensions.cs
/// <param name="action">The delegate executed when the endpoint is matched.</param>
/// <param name="httpMethods">HTTP methods that the endpoint will match.</param>
/// <returns>A <see cref="IEndpointConventionBuilder"/> that can be used to further customize the endpoint.</returns>
public static MapActionEndpointConventionBuilder MapMethods(
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need this? As a user, there are alternate ways to re-use the delegate if we need to. I'm also similarly against allowing more than one HTTP attributes per MapAction delegate.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that the flexibility of delegates means that we shouldn't need to make routing metadata overly complicated. That's why I removed IRotePatternMetadata and IRouteOrderMetadata in #30563. I also stopped implementing IHttpMethodMetadata in attributes which caused multiple instances to show up on endpoints that didn't have multiple instances before. #29878 (comment)

All that said, this is an overload that already exists for RequestDelegate and any argument about reusing the Delegate, you could make for reusing the RequestDelegate. I'm just keeping it consistent by adding a Delegate overload for every RequestDelegate overload.

@halter73 halter73 mentioned this pull request Apr 20, 2021
Closed
@amcasey amcasey added the area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc label Jun 2, 2023
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JamesNK JamesNK JamesNK left review comments

@pranavkm pranavkm pranavkm left review comments

@davidfowl davidfowl davidfowl approved these changes

@javiercn javiercn Awaiting requested review from javiercn javiercn is a code owner

Assignees
No one assigned
Labels
area-minimal Includes minimal APIs, endpoint filters, parameter binding, request delegate generator etc area-mvc Includes: MVC, Actions and Controllers, Localization, CORS, most templates feature-minimal-actions Controller-like actions for endpoint routing
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add new "MapAction" overloads
5 participants
@halter73 @davidfowl @pranavkm @JamesNK @amcasey